Improved exported paths, added NextAppRouter specific platform#418
Improved exported paths, added NextAppRouter specific platform#418lkostrowski merged 9 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 79b58b8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #418 +/- ##
==========================================
- Coverage 83.40% 81.66% -1.74%
==========================================
Files 99 107 +8
Lines 3429 3502 +73
Branches 587 593 +6
==========================================
Hits 2860 2860
- Misses 557 626 +69
- Partials 12 16 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lkostrowski
changed the title
| "types": "./util/public/browser/index.d.ts", | ||
| "import": "./util/public/browser/index.mjs", | ||
| "require": "./util/public/browser/index.js" | ||
| }, |
There was a problem hiding this comment.
chaned exports - marked these for browser and node specific, removed some because they didn't make sense anymore
| constructor(public request: Request) {} | ||
| export class WebApiAdapter< | ||
| TRequest extends Request = Request, | ||
| TResponse extends Response = Response, |
There was a problem hiding this comment.
In general:
Before: Next App Router platform was 1:1 re-exported FetchApi. But this is not correct because Next is extending it's Request to NextREquest, so it was limiting the usage (and NextResponse too)
Now: I created separated code for Next, reused WebApi where possible and injected NextRequest and NextResponse as generics
|
Released snapshot build with Install it with: pnpm add @saleor/app-sdk@0.0.0-pr-20250304145310 |
package.json
Outdated
| "types": "./verify-jwt.d.ts", | ||
| "import": "./verify-jwt.mjs", | ||
| "require": "./verify-jwt.js" | ||
| "./auth/node": { |
There was a problem hiding this comment.
Question: do we support egde runtime as well or plan to do so?
I'm asking as if we don't plan to do this we can have auth/server instead 🤔
There was a problem hiding this comment.
I think it's better if we keep node for compatibility (I dont know if it works with edge) and then if we decide to support edge, we can add alias node = edge or node = server
if we do other way around and start with "server" it will require a bc
There was a problem hiding this comment.
update: I dogfood it in Adyen and realized there is no "browser" part. JWT is from browser but it's checked on the server 🤦
So I removed node and browser and left auth
| const context = validationResult.value; | ||
| try { | ||
| return await handlerFn(request, context); | ||
| } catch (err) { |
There was a problem hiding this comment.
Question: don't we want to log error here (even if it is in debug)?
|
Released snapshot build with Install it with: pnpm add @saleor/app-sdk@0.0.0-pr-20250304160045 |
|
Released snapshot build with Install it with: pnpm add @saleor/app-sdk@0.0.0-pr-20250304161552 |
|
Released snapshot build with Install it with: pnpm add @saleor/app-sdk@0.0.0-pr-20250304162628 |
2 participants
Uh oh!
There was an error while loading. Please reload this page.